Add missing var modifier to OnCalculatePreciseCostAmountsOnAfterFilterOpenInboundItemLedgerEntry#9103
Conversation
…rOpenInboundItemLedgerEntry The event passed the open inbound item ledger entry record by value, so filters applied by subscribers were discarded before the FindSet call in CalculatePreciseCostAmounts and the event could not fulfill its purpose. The record is now passed by reference in the W1, RU, and APAC copies of codeunit ItemCostManagement, matching the sibling events in the same procedure. Adds a regression test with a bound subscriber that filters the open inbound entries and verifies the average cost reflects the filter. Fixes microsoft#7611
github-actions
Bot
added
From Fork
Agentic PR Review - Round 1Recommendation: AcceptWhat this PR doesThis PR adds the missing The publisher is raised after SuggestionsNone. Risk assessment and necessityRisk: The main risk is event compatibility, because adding Necessity: The linked GitHub issue describes a real defect: subscribers cannot add filters because the record is passed by value. Without the change, the event remains ineffective for its stated purpose. I did not find a linked ADO work item, so this assessment is based on the GitHub issue, PR description, diff, and code flow.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| end; | ||
|
|
||
| [IntegrationEvent(false, false)] | ||
| local procedure OnCalculatePreciseCostAmountsOnAfterFilterOpenInboundItemLedgerEntry(OpenInbndItemLedgerEntry: Record "Item Ledger Entry"; var Item: Record Item) |
There was a problem hiding this comment.
OnCalculatePreciseCostAmountsOnAfterFilterOpenInboundItemLedgerEntry is a public [IntegrationEvent] on codeunit 5804 ItemCostManagement, and its 'OpenInbndItemLedgerEntry: Record "Item Ledger Entry"' parameter is changed from by-value to 'var' in place (identically in the W1, APAC, and RU layers).
AL requires an event subscriber's parameter modifiers to match the publisher's exactly, so any already-shipped extension subscribing to this event with the old (non-var) signature will fail to bind/compile once it recompiles against the updated base app symbols. Modifying a published event's signature in place is a breaking change for AppSource/partner extensions. Note: were this backed by a curated rule, the real-world impact would be major/blocker (extension breakage), but per agent-finding policy severity is capped at minor here.
Recommendation:
- introduce a new event (e.g. ...WithFilterSupport, with the var parameter) and leave the existing event unchanged, or mark the existing event ObsoleteState=Pending with an ObsoleteReason/ObsoleteTag pointing subscribers at the replacement, rather than changing the parameter modifier of the existing published event.
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why
Copilot PR ReviewIteration 1 · Outcome: completed Knowledge source: https://github.com/microsoft/BCQuality@822cae1b2771ac25f665f73369f69093bd4fd630 Findings by domainFindings split into Knowledge-backed (cite a BCQuality article) and Agent (the agent's own judgement, no matching BCQuality rule).
Totals: 0 knowledge-backed · 1 agent findings. Orchestrator pre-filter (13 file(s) excluded)
Findings produced by the Copilot CLI agent against BCQuality at |
github-actions
Bot
removed
the
needs-approval
3 participants
What & why
The IntegrationEvent
OnCalculatePreciseCostAmountsOnAfterFilterOpenInboundItemLedgerEntryin codeunit 5804ItemCostManagementpassesOpenInbndItemLedgerEntryby value. It is raised right after the filters are set on the open inbound item ledger entries and right beforeFindSet(), so its whole purpose is to let subscribers refine those filters. Because the record is passed by value, any filter a subscriber applies lands on a copy and is discarded, which makes the event unusable for the very scenario it was requested for (the original event request asked for "additional filters" on the open inbound entries).This change adds the missing
varmodifier to the record parameter, in the W1 copy and in the byte-identical RU and APAC layer copies of the codeunit. All sibling events in the same procedure, and the directly analogousOnExcludeOpenOutbndCostsOnAfterOpenItemLedgEntrySetFiltersfilter hook in this codeunit, already pass their Item Ledger Entry records by reference; this event was the only exception.A regression test is added in
SCMAvgCostCalc.Codeunit.al(W1 and its CZ copy): a bound subscriber narrows the open inbound entries to a single entry and the test verifies the calculated average cost reflects only that entry. The test fails against the previous signature (the subscriber filter was ignored) and passes with this change.Linked work
Fixes #7611
How I validated this
What I tested and the outcome
varper layer copy); no executable statement changes. There are no subscribers to this event anywhere in this repository (verified by search), so no other code needed updating.PreciseAvgCostRespectsSubscriberFilterOnOpenInboundEntriesin codeunit 137070: mocks two open inbound entries (cost 6.00 over quantity 2 with remaining 1, and cost 10.00 plus rounding precision over quantity 2 with remaining 1) plus a closed outbound entry, so the total cost amount equals exactly the amount rounding precision and the precise cost path inCalculatePreciseCostAmountsis entered. A bound subscriber filters the open inbound entries down to the first entry. Expected average cost: 6.00 / 2 * 1 / 2 = 1.5. Without the fix the subscriber filter is discarded and the calculation yields 4.0025, so the test detects the defect.CalculateAverageCost,ExcludeOpenOutbndCosts,CalculatePreciseCostAmounts) against the mocked data to confirm the expected value, and mirrored the test into the CZ copy of the test codeunit to keep the two files in lockstep. Building the Base Application locally is not feasible outside the CI baselines, so I rely on the pipeline for compilation and test execution.Risk & compatibility
varis a breaking change. Adding avarmodifier on an event parameter is exactly what AppSourceCop rule AS0077 covers, and this repository deliberately sets AS0077 toNoneon main (src/rulesets/ruleset.json: "Adding a var modifier in events should be allowed in main, as it only will break the runtime behavior of extensions subscribing to it when used in hotfix scenarios"), whileminorrelease.ruleset.jsonmakes it an Error on release branches. PR Fixes #3034 #3186 fixed the same class of defect (OnBeforeGetEmailAttachmentsByEmailScenarios) with the same in-place change. This PR therefore targets main only and should not be backported toreleases/*.var(the only compilable form until now); a subscriber may keep a by-value parameter against avarpublisher parameter, so existing code keeps compiling and behaving as before. Subscribers that want the new capability opt in by declaring the parameter withvar.OpenOutbndItemLedgEntry.CopyFilters(OpenInbndItemLedgEntry)runs after the event. For the intended slice-style filters (location, variant, custom entry attributes) this is the consistent behavior, since the same restriction then applies to both the inbound and outbound legs of the precise cost calculation. Subscribers that need different filters for the outbound leg can use the existingOnCalculatePreciseCostAmountsOnBeforeProcessOpenOutboundItemLedgerEntryevent, which is raised between the two scans and already receives both records by reference.